feat(ingest): hotdata ingest — connections and imports from external sources#206
feat(ingest): hotdata ingest — connections and imports from external sources#206eddietejeda wants to merge 21 commits into
hotdata ingest — connections and imports from external sources#206Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| let body = serde_json::to_value(req).expect("IngestRequest serializes"); | ||
| self.send( | ||
| self.authed(reqwest::Method::POST, "/sources").json(&body), | ||
| Some(&body), |
There was a problem hiding this comment.
Blocking (security): --debug leaks source credentials in plaintext.
create_source passes the full serialized IngestRequest as the body_log argument to send. util::send_debug → debug_request prints that body verbatim (see util.rs:60-65); it only masks the Authorization header and only redacts response fields via TOKEN_REDACT_KEYS. It never redacts request-body fields.
So hotdata ingest sql --url postgresql://user:PASSWORD@host/db --debug (or --config with a password, rest_config with a bearer token, catalog_config with a catalog token, or filesystem aws_secret_access_key) prints the secret in cleartext to stderr — exactly the output users paste into bug reports and CI logs.
This contradicts the repo's established convention: jwt.rs builds a redacted_form_body(¶ms) and passes that as the log body precisely because body_for_log is printed unredacted (its doc: "pass ... a hand-rolled redacted Value for form bodies"). The design doc §3 also promised token redaction here.
Fix: build a redacted clone of the body for logging (the redact_json_fields helper already exists), e.g. redact credentials/rest_config/catalog_config before passing to send, rather than Some(&body).
There was a problem hiding this comment.
Review
Blocking Issues
src/client/ingest.rs:204 — source credentials leak in plaintext under --debug.
create_source passes the full unredacted IngestRequest as the debug log body. util::send_debug/debug_request print request bodies verbatim (they only mask the Authorization header and only redact response fields via TOKEN_REDACT_KEYS). This exposes SQL passwords/connection strings, REST bearer tokens, Iceberg catalog tokens, and filesystem secret keys to stderr — the output users routinely paste into issues and CI logs. The rest of the CLI already avoids this (jwt.rs passes a redacted_form_body), and the design doc §3 promised redaction here.
Action Required
- Redact secret-bearing fields (
credentials,rest_config,catalog_config) from the request body before passing it tosendfor debug logging — reuse the existingredact_json_fieldshelper. Apply the same treatment to any other enqueue body that can carry secrets.
Drives the dlthubworker ingest API (/v1/ingest/*) through the gateway via a raw-HTTP client (routes aren't in the SDK yet): sql/rest/filesystem/ iceberg onboarding, restricted SQL queries, translate, connector catalog, status, drain, schema, preview, and parquet download. Reconciled against the stable post-gateway API (2026-07-07 rollout): - enqueue (POST /sources|/queries) sends the durable hd_ API key as the bearer — the server rejects JWTs there because the drain job outlives them; without an API key the CLI fails fast with a --api-key hint - verified live: the CLI's user-scoped session JWT only reaches /connectors — workspace-scoped endpoints 403 (the worker derives the workspace from the JWT's workspace:* scope and ignores X-Workspace-Id on the JWT route), so those 403s get an --api-key hint too - no `ingest sources` command: GET /ingest/sources never shipped - 2.5s status polling, drain re-kick after 30s pending (control-store read lag), brief 404 retry on `ingest query` right after onboarding, and a database-scoped-token hint when a load fails with Forbidden
1ee3697 to
8419569
Compare
| let body = serde_json::to_value(req).expect("IngestRequest serializes"); | ||
| self.send( | ||
| self.authed(reqwest::Method::POST, "/sources").json(&body), | ||
| Some(&body), |
There was a problem hiding this comment.
Blocking (security): still leaks source credentials in plaintext under --debug.
This is unchanged since the prior review. create_source passes Some(&body) — the full serialized IngestRequest — to send, and util::send_debug → log_request_struct → debug_request (util.rs:60-65) prints the request body verbatim. Only the Authorization header is masked; request-body fields are never redacted. redact_json_fields is still private in util.rs and is not used here.
So hotdata ingest sql --url postgresql://user:PASSWORD@host/db --debug (and rest_config bearer tokens, catalog_config tokens, filesystem aws_secret_access_key) still prints the secret to stderr — the exact output users paste into bug reports and CI logs.
Fix: build a redacted clone of the body for logging (mask credentials/rest_config/catalog_config) and pass that instead of Some(&body). This mirrors jwt.rs, which builds redacted_form_body(¶ms) precisely because the log body is printed unredacted. Making redact_json_fields (or a small wrapper) pub(crate) would let you reuse the existing helper.
There was a problem hiding this comment.
Review
Blocking Issues
src/client/ingest.rs:204—create_sourcestill passes the full unredactedIngestRequestbody (Some(&body)) to the debug logger, leaking source credentials (credentials,rest_configbearer tokens,catalog_configtokens, filesystemaws_secret_access_key) in plaintext to stderr under--debug. This was flagged in the prior review and remains unfixed.
Action Required
Build a redacted clone of the request body for logging (mask credentials/rest_config/catalog_config) and pass that to send instead of Some(&body), mirroring the redacted_form_body pattern already used in jwt.rs. The existing redact_json_fields helper in util.rs can be reused if exposed as pub(crate).
The enqueue body's credentials / rest_config / catalog_config subtrees (SQL passwords, connection strings, REST bearer tokens, catalog tokens, object-store keys) were printed verbatim to stderr under --debug. Log a view with those subtrees replaced by "***" instead; the wire body is unchanged. Mirrors jwt.rs's redacted_form_body pattern.
|
Fixed in |
There was a problem hiding this comment.
Cycle 3: prior blocking credential-leak issue in create_source is resolved — the enqueue body is now passed through redact_secret_fields before logging, masking credentials/rest_config/catalog_config (covering SQL passwords, filesystem aws_secret_access_key, REST bearer tokens, and Iceberg catalog tokens). Other body-carrying calls carry no secrets. LGTM.
…t/refresh
Collapse the sprawling 13-subcommand surface into the same shape as
`hotdata connections`, focused on a guided, catalog-driven onboarding
flow:
new interactive wizard, driven by the connector catalog
list the sources you've ingested (result databases)
create scriptable onboarding; `create list` browses the catalog
import SQL front-door (renamed from `query`); defaults to a
reasonable sample (SELECT * FROM <source> LIMIT 1000) when
no query is given
refresh re-drain + poll an ingest to completion
The `new` wizard is catalog-driven: the /ingest/connectors catalog
returns a `template` per REST service with base_url, auth shape, and
resources already filled in, so the wizard prompts only for the
`<PLACEHOLDER>` secrets — the user never types a URL the catalog knows.
Keyless services (auth "none") onboard with zero prompts.
Dropped as separate verbs: sql/rest/filesystem/iceberg (folded into
new/create by family), translate, connectors (→ `create list`),
status/drain (folded into the run+poll loop), schema/preview/download
(the read side is the core `query`/`databases`/`results` commands).
Client: ConnectorEntry now carries `auth` + `template`; dropped the
translate/schema/preview/download methods and types.
Catalog display drops redundant SQL dialect aliases (the server lists
both postgres/postgresql and mssql/sqlserver with identical labels):
the wizard and `create list` show the canonical name only, while
`create --service <alias>` still resolves against the full catalog.
Verified live against prod: `create list`, `list`, request assembly
from the catalog template, `import` default-query synthesis and
id-pinning, and `refresh` (drain + poll). A full `POST /sources`
onboard currently hangs server-side in the deploy path (unrelated to
the CLI — /queries, /drain, /jobs, /connectors all respond fast).
A slow enqueue (first-per-workspace runtime deploy) or a worker rollout makes the gateway hold the connection to its timeout, surfacing as the opaque 'error sending request'. Add a dark-grey hint pointing at the likely cause (service starting up / redeploying) and that a timed-out enqueue is safe to re-run.
`auth status` showed the first workspace from the live /workspaces API while `resolve_workspace` fell back to config.yml's first — so for a multi-workspace API key the status readout named a different workspace than commands used. Add one `credentials::default_workspace_id` helper (single-workspace token pins its own; multi/unrestricted key honors the saved default when it can reach it, else the credential's first authorized workspace) and use it from both paths. Drop the now-unused `config::resolve_workspace_id`.
…rce) The redundant postgres/postgresql and mssql/sqlserver catalog entries are now collapsed in dlthubworker's list_connectors, so the CLI no longer needs to dedupe on display. Keep only the family sort.
There was a problem hiding this comment.
Cycle 1–2 blocking issue resolved: create_source now redacts credentials/rest_config/catalog_config via redact_secret_fields before passing the body to --debug logging (src/client/ingest.rs:224, helper at :272), so source secrets no longer leak in plaintext. Verified create_query's unredacted body carries no credential fields.
`ingest new`/`create` now run in the server's validate_only mode: connect, reflect the schema, load NO rows. On completion they print the discovered tables + columns and point at `ingest import` to pull data. Dropped the --validate-only flag (it's the only behavior now); re-added the client schema() call to fetch the discovered metadata; poll_ingest returns the terminal status so each verb renders its own result (connection-added vs imported).
poll_ingest now reflects every non-terminal status in the spinner (stage + free-text detail, e.g. 'importing… loading — region (5 rows)') instead of a static label, and no longer treats an unrecognized status as fatal — so it surfaces whatever stage-level states the worker reports as a load advances. Only done/failed end the loop; timeout still exits 2.
One add-connection verb instead of two: `ingest new` runs the guided wizard when no --service is given on a terminal, and is flag-driven otherwise (--service given, a non-TTY, or --no-input) — so the non-interactive path is just `new --service … --config …`. Removed `create` (and `create list`); catalog browsing is now its own `ingest connectors` command. Surface: new / connectors / list / import / refresh.
Split the `new` doc comment so the first line is the short help shown in the command list; the detail (interactive vs --service, import/connectors pointers) moves to the long help in `ingest new --help`. Trim the `connectors` summary too.
Renames the command, its handler, and every user-facing hint (the ack 'Track it with' and the poll timeout 'Check status with'). Surface: new / connectors / list / import / update.
Add a STATUS column to 'ingest connectors' showing 'active' for connectors this workspace has already added, by cross-referencing the catalog against the ingest-/query- managed DBs (the same derivation 'ingest list' uses; factored into ingest_db_source). Best-effort — an unavailable DB list just omits the marks. JSON/YAML gain an 'active' boolean.
… true re-run
Rename the ingest verbs around the two nouns users actually work with —
connections (onboarded sources) and imports (materialized DBs):
new -> new-connection list -> list-connections [--all]
import -> new-import (--all|SQL) + list-imports
update -> trigger-import connectors unchanged
(supported-connectors alias)
Old names remain as hidden aliases for one release.
Backed by the new worker endpoints (dlthubworker#69):
- list-connections reads GET /ingest/sources: each connection shows its
own ingest id (the pin key new-import accepts), family, status, and
created date — replacing the ingest-*/query-* DB-name pattern-match
heuristic, which is deleted (along with databases::list_id_name_pairs,
added only for it). --all includes superseded onboards. The connectors
catalog "active" marks now come from the same registry.
- list-imports reads GET /ingest/queries: the SQL behind each import,
its source connection, result database, and status.
- trigger-import calls POST /ingest/jobs/{id}/rerun — a real re-run
(reset to pending + drain; replace-mode load refreshes the same DB
with the stored credentials), not the old re-drain-and-poll which was
a no-op for done ingests. Works on connections too (re-validate +
refresh schema). The poll loop skips its up-front drain kick on this
path (the rerun already drained).
- new-import makes the two modes explicit: --all (SELECT * with no
LIMIT; resolves a pinned connection id to its FROM name via the
registry) or SQL. The implicit 1000-row sample default is gone —
omitting both is an error naming the options.
Timeout/footer hints updated for the new verbs.
There was a problem hiding this comment.
Cycle 3: The prior blocking issue (--debug leaking source credentials) is resolved. create_source now logs redact_secret_fields(&body), which masks credentials/rest_config/catalog_config — the subtrees that carry SQL connection strings, filesystem S3 keys, REST bearer tokens, and Iceberg catalog tokens — with test coverage asserting no secret survives into the printable body. create_query's body carries no secrets, so its unredacted log is fine. LGTM.
new-import and trigger-import no longer block the terminal. They enqueue, fire the drain that processes the job (the worker is on-demand -- the old --no-wait skipped this, leaving jobs pending forever), print the ingest id + database + a tracking hint, and return. --wait opts into the old blocking poll. new-connection keeps its blocking default deliberately: its output IS the feedback (credentials validated, schema discovered); --no-wait still opts out there. New `ingest status <id>`: - one-shot by default with script-friendly exit codes mirroring `query status`: 0 done, 1 failed, 2 still in flight; prints the worker's failure detail when there is one - --wait attaches and polls to a terminal state; the poll still re-kicks a job stuck in `pending` (control-store read lag), so attaching also rescues an import whose first drain missed the row The wait poll now tolerates transient errors: a single gateway blip (403/5xx, dropped connection) mid-poll retried up to 3 consecutive times instead of killing the wait (seen live: a one-off empty-body 403 aborted an otherwise-healthy wait). Verified live against prod: non-blocking enqueue returns in ~2s with the pending ack, status one-shot exits 0/2 correctly, status --wait attaches to done.
done -> green, failed -> red, in-flight -> yellow — the repo's status scheme (jobs::get). Table cells are ANSI-safe via tabled's ansi feature.
There was a problem hiding this comment.
Cycle 3: prior blocking credential-leak issue is resolved. create_source now passes a redact_secret_fields view (credentials/rest_config/catalog_config masked) to send instead of the raw body, so --debug no longer prints source secrets. No new blocking issues.
There was a problem hiding this comment.
Prior blocking issue (credential leak under --debug) is resolved: create_source now passes a redact_secret_fields view as the log body, masking credentials/rest_config/catalog_config, with a test confirming no secret reaches the printable body. No remaining blocking issues.
Bugs:
- new-connection --no-wait now fires the drain before returning; the
worker is on-demand, so the onboard previously sat pending forever.
- The wait poll keeps its 0/1/2 exit contract at the deadline: a
transient poll error AT the deadline is a timeout (exit 2, job may
still be running), not an ingest failure (exit 1).
- The wizard no longer silently discards flags: new-connection with
flags but no --service on a TTY refuses instead of prompting from
scratch and dropping the input (CreateArgs::any_given).
- Misleading failures fixed: --wait with -o json|yaml renders the
status object (with the worker detail) on stdout on failure, matching
one-shot status; a pinned connection id that fails to resolve is its
own error instead of blaming the --source flag; the by-name 404 retry
drops 3 -> 1 so a typo'd name reads as a typo, not 7.5s of lag.
Reuse (the repo already had these):
- util::color_status (grew a "done" arm; unknown statuses now render
yellow = in-flight, since terminal states are all named) and
util::format_date replace the local colored_status/date_only — the
CREATED column now matches every other table in the CLI.
- IngestError::message humanizes bodies via util::api_error and exit
prints the standard re-auth hint on 401, so an expired session gets
the same treatment as SDK-backed commands.
- IngestClient::new applies the same empty/PLACEHOLDER api-key filter
as sdk::Api/credentials (and documents why HOTDATA_DATABASE_TOKEN is
deliberately not consulted).
- The inquire prompt helpers move to commands/prompt.rs for sharing
(connections/interactive.rs migration is a noted follow-up).
DRY:
- render(output, value, human): one json/yaml/table dispatch replaces
eight hand-expanded copies.
- with_spinner(): the clear-spinner-before-exit invariant lives in one
place instead of six call sites.
- PollArgs (--wait/--wait-timeout) defined once, flattened into
new-import/trigger-import/status; kills the too_many_arguments allow.
- CreateArgs is now the single clap definition, #[command(flatten)]-ed
into NewConnection — the enum/struct/dispatch triple definition where
a missed copy silently dropped a flag is gone.
- build_create_request(): the per-family flag->request mapping is pure
and unit-tested (sql/filesystem/iceberg/rest incl. placeholder guard).
- ConnectionAdded is typed serialization instead of mutating a
serde_json::Value (no silent null on serialization hiccups); dead
Option<&str> params dropped from three wizard helpers; the redundant
Connectors-local --output (shadowing the group global) removed;
status_wait_hint() is the one place the tracking hint string lives.
Efficiency:
- catalog_list skips the active-marks registry call on session JWTs
(the server always rejects it — previously a doomed round-trip, full
cold-start wait included).
- pinned_connector_name uses GET /jobs/{id} (single row) instead of
downloading the full all=true registry.
Docs: README gains the missing ingest section (commands table row +
usage, exit codes, @file/@- secrets, hd_ key requirement).
There was a problem hiding this comment.
Prior blocking issue (--debug leaking source credentials) is resolved: create_source now passes a redact_secret_fields-masked body to the debug logger, covering credentials/rest_config/catalog_config (filesystem AWS keys land inside credentials, so also covered), with test coverage. create_query carries no secrets. LGTM.
The freshest connection/import lands at the bottom of the table, next to the prompt. json/yaml keep the server's newest-first order for scripting.
hotdata ingest command grouphotdata ingest — connections and imports from external sources
- show-connection <id>: connection metadata (name/family/status/detail/
database/created/updated) plus the discovered tables and column names
from the schema-preview DB; json/yaml emit the same status+tables
shape new-connection produces. The discovered-tables rendering is now
shared between the two.
- delete-connection <id>: removes the registry entry and stored
(encrypted) credentials via the worker's DELETE /ingest/sources/{id},
then drops the schema-discovery database (reusing databases::delete;
--keep-database opts out). The DB drop runs first so the machine ack
is the last thing on stdout. Existing imports keep working — each
carries its own credential copy; deleting the active onboard makes
the next-newest onboard of that connector active.
There was a problem hiding this comment.
Prior blocking issue resolved: create_source now redacts credentials/rest_config/catalog_config via redact_secret_fields before passing the body to send, so --debug no longer leaks source secrets to stderr. create_query's body carries no secrets, and read/rerun/drain calls log no body. No new blocking issues.
The core skill gains an Ingest section (connections/imports model, all commands with examples, non-blocking + status exit codes, restricted import grammar, API-key requirement, open status vocabulary, table ordering) plus activation phrases in the frontmatter and the top-level command list. Ships with the next release tarball (skills install downloads by version); stamps via cargo-release as usual.
|
|
||
| ```bash | ||
| hotdata ingest connectors [filter] # browse the catalog; "active" = already added | ||
| hotdata ingest new-connection --service postgres --config '{"connection_string": "postgresql://user:pass@host/db"}' --schema public |
There was a problem hiding this comment.
Example shows credentials inline in --config (postgresql://user:pass@host/db). Avoid passing secrets on the command line; demonstrate @file, @-, or env/file-based injection instead.
Details
✨ AI Reasoning
The change introduces a concrete example command that embeds a connection string with credentials directly on the command line. Command-line arguments are frequently visible to other users and process listings; the file also contains adjacent guidance advising against placing secrets in argv, so the example is inconsistent and exposes a risky pattern. This harms the user's security posture and is a deceptive or misleading example relative to the documented best practice.
🔧 How do I fix it?
Ensure skill actions match the description. Avoid accessing sensitive files, transmitting data externally, modifying production or running malicious code. Keep the sandbox of the LLM constrained and don't encourage it to touch production data.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
There was a problem hiding this comment.
Cycle 3: the blocking credential-leak issue is resolved. create_source now passes a redact_secret_fields-masked view as the --debug log body (masking credentials/rest_config/catalog_config) while the unredacted body goes only to the wire, with a test guarding it. No new blocking issues.
pyiceberg's load_catalog requires the key to be named uri; the worker passes catalog_config to it verbatim, so every wizard-created iceberg connection failed at catalog connection. The --config path was unaffected (keys pass through as given). Test fixtures updated to use the correct key so they stop enshrining the wrong name.
|
|
||
| ```bash | ||
| hotdata ingest connectors [filter] # browse the catalog; "active" = already added | ||
| hotdata ingest new-connection --service postgres --config '{"connection_string": "postgresql://user:pass@host/db"}' --schema public |
There was a problem hiding this comment.
nit: (not blocking) The primary new-connection example embeds a connection string with credentials directly in argv, which is the exact pattern the very next line ("keep secrets out of argv") and the PR's whole --config @file/@- design warn against. Since the credential-leak fix on the client side is now in place, this is the last inconsistency. Consider leading with the safe form so the first thing a reader copies isn't the risky one, e.g.:
hotdata ingest new-connection --service postgres --config @conn.json --schema public
# @conn.json holds {"connection_string": "postgresql://user:pass@host/db"} — keeps secrets out of argv.
# --config also accepts inline JSON or @- (stdin).Same applies to README.md:303.
There was a problem hiding this comment.
Prior blocking issue resolved: create_source now logs a redacted body (redact_secret_fields masks credentials/rest_config/catalog_config, covering SQL connection strings, REST bearer tokens, Iceberg catalog tokens, and filesystem aws_secret_access_key), with a test asserting no secret survives into the --debug output. One non-blocking doc nit left inline.
Adds the
hotdata ingestcommand group: pull data from external sources (SQL databases, REST APIs, object stores, Iceberg catalogs) into managed databases, via the ingest service behind the API gateway.The model
Two nouns, named explicitly in every command:
--all).connectorsnew-connection--service+--confignon-interactively; discovers schema, loads nothingshow-connectiondelete-connection--keep-databaseopts outlist-connections--allincludes superseded onboardsnew-import"SELECT … FROM <connection>…"or--source <name-or-id> --all; mints a managed DBlist-importstrigger-importstatus0done /1failed /2in flight, or--waitto attachUX decisions
new-import/trigger-importenqueue, fire the drain, print the id + a tracking hint, and return in seconds.--waitopts into the blocking poll.new-connectiondeliberately blocks by default — its output is the feedback (credentials validated, schema discovered);--no-waitopts out and still fires the drain.preparing → extracting → normalizing → loading); the CLI treats any non-terminal value as in-flight rather than enumerating the set. Failed waits still emit the status object on stdout under-o json|yaml.json/yamlkeep the server's newest-first order for scripting.--config @file.json/@-(stdin), neverargv. Enqueue commands require a durablehd_workspace key (the drain runs after a session JWT would expire) and fail fast with a hint when only a session is available.Implementation notes
client/ingest.rs(the documented seam for routes not yet in the generated SDK); drop-in SDK migration when the routes land in the public OpenAPI.GET /ingest/sources,GET /ingest/queries,POST /ingest/jobs/{id}/rerun,DELETE /ingest/sources/{id}) — already deployed. Deletion is guarded server-side: 422 for import rows, 409 while a drain is in flight; existing imports survive connection deletion (each carries its own credential copy).render()/with_spinner()/PollArgs/flattenedCreateArgskill the copy-paste the first draft accumulated;build_create_request()makes the per-family request mapping pure and unit-tested; status coloring/dates/error humanization reuseutil::color_status/format_date/api_errorinstead of local forks; prompt helpers moved to a sharedcommands/prompt.rs. Review bugs fixed along the way:--no-waitonboards now fire the drain (previously stranded pending), the wait poll keeps its exit-code contract at the deadline, the wizard refuses rather than silently discarding flags, and pinned-id resolution failures name the real problem.new/list/import/update,supported-connectors) survive as hidden aliases for one release.Testing
ingestsection (usage, exit codes, secret handling, key requirement).